Warn on incomplete concrete classes that inherit from abstract classes#7955
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
0652845 to
855c354
Compare
This comment has been minimized.
This comment has been minimized.
|
Updated the test refs, but I'm not sure they're right. I'm getting lots of diffs for a lot of tests. Running pylint built from main on Windows 11 ( It appears I'm getting the correct output with Ubuntu 20.04, but |
|
Should be good now, let me know if there's any issues @DanielNoord! I had to add a print into the tester to get the results :/ |
This comment has been minimized.
This comment has been minimized.
|
@DanielNoord is there anything else needing to be done? |
The CI fails so we would first need to fix that before this can be merged. |
|
For whatever reason, both Python 3.8 and 3.10 do not work to update the abstract method file. I'm running self = <pylint.testutils.functional.lint_module_output_update.LintModuleOutputUpdate object at 0x7fa1e3c6d690>
def runTest(self) -> None:
> self._runTest()
E AssertionError: Wrong results for file "abstract_method":
E
E Unexpected in testdata:
E 47: abstract-method
../pylint/testutils/lint_module_test.py:145: AssertionError
=========================================================================================================================================== short test summary info ===========================================================================================================================================
FAILED test_functional.py::test_functional[abstract_method] - AssertionError: Wrong results for file "abstract_method":
============================================================================================================================ 1 failed, 779 passed, 25 skipped in 62.76s (0:01:02) ============================================================================================================================= |
|
Could it be that the test simply fails? If haven't checked that myself locally |
|
Do you have any docs for what the comments do? For example, I'm not sure if Are the functional tests a sort of blend of replay with the file outputs and unit tests? |
This (attempts to) describe the functional test framework. Basically it tries to lint the file and sees where we expect messages to be raised and compares that against the actual output of the linting. |
|
@sshane This is quite close to being done. Do you have intention of working on it? Or shall we close it? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7955 +/- ##
=======================================
Coverage 96.22% 96.22%
=======================================
Files 178 178
Lines 19699 19700 +1
=======================================
+ Hits 18956 18957 +1
Misses 743 743
🚀 New features to boost your workflow:
|
This comment has been minimized.
This comment has been minimized.
|
@DanielNoord I'm having the same problem I did when I last worked on it, I verified my Running |
|
Narrowed it down to #9688. I don't have any more time to spend on this for the time being, feel free to get it across the line if it's simple for you, else you can close. |
This comment has been minimized.
This comment has been minimized.
|
I was expecting each line's warnings to be compared to the annotations, not one by one! A more clear error message would have been nice 🙂 |
This comment has been minimized.
This comment has been minimized.
|
@DanielNoord I added the count which would have surfaced this for me much quicker, let me know if it's something you want and I can split it out |
This comment has been minimized.
This comment has been minimized.
DanielNoord
left a comment
There was a problem hiding this comment.
Only needs a news entry using towncrier. I'm fine with merging without full coverage.
``class_is_abstract`` previously treated a class as abstract whenever any of its ancestors inherited from ``abc.ABC``. That meant a concrete class which only inherited from an abstract intermediate (without re-declaring ``abc.ABC`` or ``ABCMeta``) was silently exempted from the ``abstract-method`` check, even when it left abstract methods unimplemented. The check now only treats a class as abstract when it declares so itself: direct ``abc.ABC`` inheritance, ``metaclass=ABCMeta``, an ``@abstractmethod`` defined on the class, or a Protocol class. Concrete subclasses that skip the explicit opt-in are now flagged for the abstract methods they inherit and do not override. Closes pylint-dev#7950
334651c to
9021d25
Compare
|
As incredible as it seems, every new primer warning I checked seem to be an actual issue. |
- Bump main reference to d523e3e - Mark pylint-dev#7950, pylint-dev#6211, pylint-dev#3716 FIXED in triage_state.json + issues_raw.json (closed by PRs pylint-dev#7955, pylint-dev#7360, pylint-dev#10989) - BRANCH_PRIORITIZATION.md: new "Landed since last snapshot" table, swap takeover-7611 and takeover-py315-support into P0 (takeover-7360 archivable now that pylint-dev#7360 merged), refresh recommended order and next-moves - FIXED_AUDIT.md: log the 3 new closures and refreshed tally - ISSUE_TRIAGE.md: note the 1022→1017 open-count refresh Verdicts now: DESIGN 596 / EXTDEP 197 / REPRO 180 / FIXED 32 / UNCLEAR 9 / DUP 2 / STALE 1 (1017 open).
Fixes: #7950
TODO: